• @0x4007 @rndquu can I get some feedback? I will start writing tests for the AMO minter and for the Aave AMO, therefore any core feedback would be invaluable until then. Regarding code style I will adhere to it as a last step, don't worry about it.

• @0x4007 @rndquu can I get some feedback? I will start writing tests for the AMO minter and for the Aave AMO, therefore any core feedback would be invaluable until then. Regarding code style I will adhere to it as a last step, don't worry about it.

Hey, sorry for a late response, I'll check it today

@zugdev

Notes:

1. AaveAMO doesn't really use UbiquityAMOMinter (although defines it in the constructor) so it's not clear how those 2 contract will interact with each other and UbiquityPool.
2. UbiquityAMOMinter is basically a fork of FraxAMOMinter (which is expected). We need only methods related to collateral moving to and from AMO strategies.
3. All aave related structs/interfaces/contracts should be taken from aave github repository (i.e. aave should be installed in github submodules) instead of copy/pasting them.
4. There is no need in copy/pasting TransferHelper lib, just use SafeERC20.

Question:

• What contract did you use as a base one from the Frax github repository for the AaveAMO contract?

1. and 2. - was waiting for this specific clarification to resolve.

2. and 4. - understood.

What contract did you use as a base one from the Frax repo for the AaveAMO

Used AaveAMO_V2

Yes, there is still some stuff to be tuned, will wrap it this week.

Yo it should be well directed now. I've:

1. Refactored Aave AMO from V2 to V3, most liquidity is there.
2. Implemented rewards collection mechanism.
3. Using contracts from git submodule instead of copypasted interfaces.
4. Refactored code to use collateral only.
5. Implemented emergency mechanisms: arbitrary calls and ERC20 recovery for owner - your choice to keep.

The AMO has the minter address to return collateral. I have implemented borrow and repay as well, this could be kept or removed - its a trust decision.

I still have to write tests and addapt code style, will be pushing this when I get some free time.

A little more context on 5: to handle rewards, borrows and repays, the system would need some way to swap, could design a Uniswap AMO. There could already be something in place, I haven't checked if it exists as a facet.

Cowswap is preferred if that's an option!

Cowswap is preferred if that's an option!

Can do that! Let me wrap this one first though.

@rndquu I've marked this PR as ready for review. Please let me know if it is up to your standards. I agree an issue to write a non-forked test suite is appropriate, I would be down to go through with it.

• AmoMinter owner should be able to transfer tokens to aave in a single tx (right now owner has to execute a 2nd tx to deposit collateral to aave)

There are a couple ways to do that. Adding AMO contracts to the main diamond or a specific AMO diamond is one of them. If so the IAmo concept will be extinguished, as function names can't be shared.

• IAmo could have deposit() and withdraw() methods (which fits at least with lending and DEX protocols)

Another oposing idea could be adhering to this IAmo idea, keep standalone AMO contracts and implement overriden deposit and withdraw methods so that function giveCollateralToAmo receives a fixed set of arguments. I am against this approach as the arguments passed in depositing/withdrawing would become static, limiting a lot possivle strategy branches - as different deposit functions require different arguments. But it is indeed an option.